Fix non-determinism with new feature resolver. #8701
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I can't say how many times I've flip-flopped on which approach to take. This isn't awful, but it's not great. |
|
It sounds good from the description. :-) |
ehuss
force-pushed
the
unique-unit-dep-hash
branch
from
cd52bcd
to
9efa0d5
Compare
|
I agree that this isn't the greatest solution, but I can't think of anything better. The best alternative I can think of is to silently translate the output directory for One thing I wanted to make sure of though, this is hashing dependencies and to confirm, our lists of dependencies are deterministic right? That is, the hashes are still deterministic? I know we need to be careful with all the |
I believe so, did you have specific thoughts of what could be a potential source of non-determinism? I think As for the list of which dependencies to use, if that was not stable, there would be bigger problems. The order is always sorted. |
|
Oh I was mostly worried about the hash field which is also hashed. We'll need to hash things always in the same order, but I think that's always true because all the dependencies are always sorted. @bors: r+ |
|
📌 Commit 9efa0d5 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 6 commits in 875e0123259b0b6299903fe4aea0a12ecde9324f..8777a6b1e8834899f51b7e09cc9b8d85b2417110 2020-09-08 20:17:21 +0000 to 2020-09-15 19:11:03 +0000 - updated yank error message (rust-lang/cargo#8697) - Fix non-determinism with new feature resolver. (rust-lang/cargo#8701) - Display formatted output for JSON diffing in tests. (rust-lang/cargo#8692) - Add --name suggestion for cargo new (rust-lang/cargo#8675) - Sweep unrelated message from unnecessary workspace infromation (rust-lang/cargo#8681) - Docs: Make it more clear we have two types of workspaces (rust-lang/cargo#8666)
This fixes a problem where Cargo was getting confused when two units were identical, but linked to different dependencies. Cargo generally assumes
Unitis unique, but the new feature resolver can introduce a situation where two identicalUnits need to link to different dependencies. In particular, when building without the--targetflag, the difference between a host unit and a target unit is not captured in theUnitstructure. A dependency shared between normal dependencies and build dependencies can need to link to a second shared dependency whose features may be different.The solution here is to build the unit graph pretending that
--targetwas specified. Then, after the graph has been built, a second pass replacesCompileKind::Target(host)withCompileKind::Host, and adds a hash of the dependencies to theUnitto ensure it stays unique when necessary. This is done to ensure that dependencies are shared if possible.I did a little performance testing, and I couldn't measure an appreciable difference. I also ran the tests in a loop for a few hours without problems.
An alternate solution here is to assume
--target=hostif--targetisn't specified, and then have some kind of backwards-compatible linking in thetargetdirectory to retain the old directory layout. However, this would result in building shared host/normal dependencies twice. For most projects, this isn't a problem. This already happens when--targetis specified, or--releaseis used (due to #8500). I'm just being very cautious because in a few projects this can be a large increase in build times. Maybe some day in the future we can be more bold and force this division, but I'm a little hesitant to make that jump.Fixes #8549